Benchmark Results

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@0x0f0f0f the benchmarks look good!

I just tested this again with multiple threads (julia -t 12).

using Metatheory

function run_eq()
  theory = @theory a b c begin
            a + b == b + a
            a * b == b * a
            a + (b + c) == (a + b) + c
            a * (b * c) == (a * b) * c
  end

  g = EGraph{Expr}(:(1 + (2 + (3 + (4 + (5 + 6))))));
  params = SaturationParams(timeout=100)
  report = saturate!(g, theory, params)
end

function test_threads()
    println("thread pool size: $(Threads.threadpoolsize())")
    Threads.@threads for i in 1:1000
        run_eq()
    end
end

The issue does not seem to be resolved. Usually the error when accessing one of the dictionaries in the EGraph.

julia> test_threads()
thread pool size: 12
ERROR: TaskFailedException

    nested task error: KeyError: key VecExpr(UInt64[0xc7970979e9cc1f1a, 0x0000000000000011, 0x4079ccad1b3303eb, 0x6b268fa6aca9af40, 0x0000000000000006, 0x0000000000000005]) not found
    Stacktrace:
      [1] getindex(h::Dict{Metatheory.EGraphs.IdKey, EClass{Nothing}}, key::Metatheory.EGraphs.IdKey)
        @ Base ./dict.jl:498 [inlined]
      [2] add!(g::EGraph{Expr, Nothing}, n::VecExpr, should_copy::Bool)
        @ Metatheory.EGraphs ~/.julia/dev/Metatheory/src/EGraphs/egraph.jl:276
      [3] instantiate_enode!(bindings::SubArray{UInt128, 1, Vector{…}, Tuple{…}, true}, g::EGraph{Expr, Nothing}, p::PatExpr)
        @ Metatheory.EGraphs ~/.julia/dev/Metatheory/src/EGraphs/saturation.jl:137
      [4] apply_rule!(bindings::SubArray{…}, g::EGraph{…}, rule::EqualityRule, id::UInt64, direction::Int64)
        @ Metatheory.EGraphs ~/.julia/dev/Metatheory/src/EGraphs/saturation.jl:174
      [5] eqsat_apply!(g::EGraph{…}, theory::Vector{…}, rep::Metatheory.EGraphs.SaturationReport, params::SaturationParams, ematch_buffer::OptBuffer{…})
        @ Metatheory.EGraphs ~/.julia/dev/Metatheory/src/EGraphs/saturation.jl:242
      [6] macro expansion
        @ ~/.julia/packages/TimerOutputs/Lw5SP/src/TimerOutput.jl:237 [inlined]
      [7] eqsat_step!(g::EGraph{…}, theory::Vector{…}, curr_iter::Int64, scheduler::Metatheory.EGraphs.Schedulers.BackoffScheduler, params::SaturationParams, report::Metatheory.EGraphs.SaturationReport, ematch_buffer::OptBuffer{…})
        @ Metatheory.EGraphs ~/.julia/dev/Metatheory/src/EGraphs/saturation.jl:287
      [8] saturate!(g::EGraph{Expr, Nothing}, theory::Vector{RewriteRule}, params::SaturationParams)
        @ Metatheory.EGraphs ~/.julia/dev/Metatheory/src/EGraphs/saturation.jl:324
      [9] run_eq()
        @ Main ./REPL[17]:11
     [10] macro expansion
        @ ./REPL[16]:4 [inlined]
     [11] (::var"#1525#threadsfor_fun#154"{var"#1525#threadsfor_fun#153#155"{UnitRange{Int64}}})(tid::Int64; onethread::Bool)
        @ Main ./threadingconstructs.jl:214
     [12] #1525#threadsfor_fun
        @ Main ./threadingconstructs.jl:181 [inlined]
     [13] (::Base.Threads.var"#1#2"{var"#1525#threadsfor_fun#154"{var"#1525#threadsfor_fun#153#155"{UnitRange{Int64}}}, Int64})()
        @ Base.Threads ./threadingconstructs.jl:153

...and 10 more exceptions.

Stacktrace:
 [1] threading_run(fun::var"#1525#threadsfor_fun#154"{var"#1525#threadsfor_fun#153#155"{UnitRange{Int64}}}, static::Bool)
   @ Base.Threads ./threadingconstructs.jl:171
 [2] macro expansion
   @ Main ./threadingconstructs.jl:219 [inlined]
 [3] test_threads()
   @ Main ./REPL[16]:3
 [4] top-level scope
   @ REPL[18]:1 

I do not understand exactly how the rules are compiled, but I guess that the compiled rules (generated functions) are shared over all threads and I suspect that the generated patterns are shared as well. This means that updating the pattern nodes is a race condition and not allowed. For example as here:

function instantiate_enode!(bindings, @nospecialize(g::EGraph), p::PatLiteral)::Id
  add_constant_hashed!(g, p.value, v_head(p.n))
  add!(g, p.n, true)
end

instantiate_enode!(bindings, @nospecialize(g::EGraph), p::PatVar)::Id = v_pair_first(bindings[p.idx])
function instantiate_enode!(bindings, g::EGraph{ExpressionType}, p::PatExpr)::Id where {ExpressionType}
  add_constant_hashed!(g, p.head, p.head_hash)

  for i in v_children_range(p.n)
    @inbounds p.n[i] = instantiate_enode!(bindings, g, p.children[i - VECEXPR_META_LENGTH])
  end
  add!(g, p.n, true)
end

function instantiate_enode!(bindings, g::EGraph{Expr}, p::PatExpr)::Id
  add_constant_hashed!(g, p.quoted_head, p.quoted_head_hash)
  v_set_head!(p.n, p.quoted_head_hash)

  for i in v_children_range(p.n)
    @inbounds p.n[i] = instantiate_enode!(bindings, g, p.children[i - VECEXPR_META_LENGTH])
  end
  add!(g, p.n, true)
end

Should be easy to test if this is the problem by just creating new VecExprs immediately instead of cloning them in add!()

I do not understand exactly how the rules are compiled, but I guess that the compiled rules (generated functions) are shared over all threads and I suspect that the generated patterns are shared as well. This means that updating the pattern nodes is a race condition and not allowed. For example as here:

function instantiate_enode!(bindings, @nospecialize(g::EGraph), p::PatLiteral)::Id
  add_constant_hashed!(g, p.value, v_head(p.n))
  add!(g, p.n, true)
end

instantiate_enode!(bindings, @nospecialize(g::EGraph), p::PatVar)::Id = v_pair_first(bindings[p.idx])
function instantiate_enode!(bindings, g::EGraph{ExpressionType}, p::PatExpr)::Id where {ExpressionType}
  add_constant_hashed!(g, p.head, p.head_hash)

  for i in v_children_range(p.n)
    @inbounds p.n[i] = instantiate_enode!(bindings, g, p.children[i - VECEXPR_META_LENGTH])
  end
  add!(g, p.n, true)
end

function instantiate_enode!(bindings, g::EGraph{Expr}, p::PatExpr)::Id
  add_constant_hashed!(g, p.quoted_head, p.quoted_head_hash)
  v_set_head!(p.n, p.quoted_head_hash)

  for i in v_children_range(p.n)
    @inbounds p.n[i] = instantiate_enode!(bindings, g, p.children[i - VECEXPR_META_LENGTH])
  end
  add!(g, p.n, true)
end

Should be easy to test if this is the problem by just creating new VecExprs immediately instead of cloning them in add!()

True! @gkronber - we could try adjusting this in this PR

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.22%. Comparing base (0b2e6c9) to head (52b4846).
Report is 6 commits behind head on ale/3.0.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@             Coverage Diff             @@
##           ale/3.0     #244      +/-   ##
===========================================
+ Coverage    81.06%   81.22%   +0.16%     
===========================================
  Files           19       19              
  Lines         1500     1513      +13     
===========================================
+ Hits          1216     1229      +13     
  Misses         284      284              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gkronber do you mind contributing a small MWE of the problem to this branch as a test?

Problem occurrs only in multi-threaded scenarios. I'm not sure how I could enforce an environment with multiple threads for a single test case. But I can have a look. Any hints are appreciated.

While the race conditions for accessing p.n in instantiate_node! are easy to fix, this is not the only problem. The nodes in patterns are also accessed within the compiled matching functions (see bind_expr) and from lookup_pat.

Problem occurrs only in multi-threaded scenarios. I'm not sure how I could enforce an environment with multiple threads for a single test case. But I can have a look. Any hints are appreciated.

@gkronber We have to set JULIA_NUM_THREADS in CI config

I added a test that fails because of a race condition and changed the number of threads for the CI pipeline.

Interestingly, 5398503 was sufficient to solve the problem for me. I've the feeling that it is only a partial fix, because the nodes in the patterns are shared and mutated from lookup_pattern as well as in ematching.

Interestingly, 5398503 was sufficient to solve the problem for me. I've the feeling that it is only a partial fix, because the nodes in the patterns are shared and mutated from lookup_pattern as well as in ematching.

Why would we ever want to mutate the pattern? That seems fundamentally ill-advised...

Interestingly, 5398503 was sufficient to solve the problem for me. I've the feeling that it is only a partial fix, because the nodes in the patterns are shared and mutated from lookup_pattern as well as in ematching.

Why would we ever want to mutate the pattern? That seems fundamentally ill-advised...

@olynch that was an optimization to not allocate memory all the times, do you have any suggestion?

Could we maybe add some spare memory, indexed by thread id, to each pattern? Then that spare memory could be used in each thread to avoid allocation, and we wouldn't need to mutate the pattern?